Ignore enhancements #454
Ignore enhancements #454
Conversation
Sounds reasonable overall, but I actually lean towards a hard fail. You could just throw a At the very least we should report a warning. This package doesn't really have any logging infrastructure yet though, so maybe just print a warning message to stderr. The cleaner solution would be to use package:logging, if you don't mind the extra effort. |
|
@liamappelbe hard fail sounds good to me. Seems like it would be more obvious what the issue is in CI environments, especially with noisy logs. I'll move forward with adding the |
|
Changed to throwing |
test/util_test.dart
Outdated
| for (final content in invalidSources) { | ||
| expect(getIgnoredLines(content.split('\n')), isEmpty); | ||
| test('throws FormatException when the annotations are not balanced', () { | ||
| for (var i = 0; i < invalidSources.length; ++i) { |
There was a problem hiding this comment.
The structure of this test is a bit odd. Can you refactor it like this:
test('throws FormatException when the annotations are not balanced', () {
runTest(String source, String errMsg) {
final content = invalidSources[i].split('\n');
expect(...);
}
runTest(invalidSources[0], 'coverage:ignore-start found at content-0.dart:3'
' before previous coverage:ignore-start ended');
runTest(invalidSources[1], 'coverage:ignore-start found at content-1.dart:3'
' before previous coverage:ignore-start ended');
...
});
* typo and case fixes * allow omitting space between // and coverage * allow text after coverage:ignore comments * handle unbalanced ignore ranges instead of bailing * add changelog entry * update title in changelog * throw FormatException when encountering unbalanced ignore comments * add file path to err message and test err message * refactor test
Adds some enhancements to ignore comment handling:
//andcoveragein coverage ignore commentsFormatExceptionwhen encountering unbalanced ignore comments instead of silently erroringHopefully 1 and 2 are uncontroversial.
3 did change an existing test.
We found it quite confusing in our projects when seemingly ignored lines were still appearing as uncovered.
Only after much investigation would various devs discover that it was due to accidental nesting of ignore ranges.
To my mind, the coverage tool should lean towards being permissive (or maybe hard fail?).
Potentially an analyzer rule could be added to detect unbalanced coverage ignore comments.
Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.